Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expander Fixes #4394

Merged
merged 23 commits into from
Mar 26, 2021
Merged

Expander Fixes #4394

merged 23 commits into from
Mar 26, 2021

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Mar 2, 2021

Description

Closes #4390
Closes #4391
Closes #4248 (The Header is now stretched by default and will respect min/max/width)

Motivation and Context

  1. Fixes 'Expanding' event to be named 'Expanded' which follows convention
  2. Ensures events are fired after the state is applied
  3. The control is now responsive. Unlike all other content controls, the Expander previously set a MaxWidth in the default style directly. This prevents the control from stretching horizontally.
  4. The control now resizes to fit content as all other content controls such as Button, Border, Grid do.

How Has This Been Tested?

Manually tested in MUXControlsTestApp

Screenshots (if appropriate):

gifexample2

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 2, 2021
robloo added 2 commits March 2, 2021 00:26
The header and content container elements should always be stretched to give child controls maximum available area.
@robloo
Copy link
Contributor Author

robloo commented Mar 2, 2021

I think there are more issues with the default Expander style. This should be reviewed more carefully, I only fixed the ones I noticed. I'm certain all the height/width settings are not optimal. The control needs to be much more responsive in its architecture and I only did a quick pass on fixing this. (Stretching vertically is still likely not supported for example)

dev/Expander/Expander.idl Outdated Show resolved Hide resolved
@ranjeshj ranjeshj added area-Expander team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 2, 2021
dev/Expander/Expander.xaml Show resolved Hide resolved
dev/Expander/Expander.xaml Outdated Show resolved Hide resolved
dev/Expander/Expander.xaml Outdated Show resolved Hide resolved
dev/Expander/Expander.xaml Outdated Show resolved Hide resolved
robloo added 4 commits March 2, 2021 14:52
This aligns with the default Button style
This aligns with the default Button style
These should follow the layout root (parent grid) sizing.
@robloo
Copy link
Contributor Author

robloo commented Mar 2, 2021

Setting a height for this control is a little strange when the expander is collapsed (see below). Should the Header take all of this space in this case? Edit: WPF behaves the same so nevermind.

(UWP)
image

image

(WPF)
image

dev/Expander/Expander.xaml Outdated Show resolved Hide resolved
Comment on lines 138 to 140
HorizontalAlignment="Stretch"
HorizontalContentAlignment="Stretch"
VerticalContentAlignment="Center"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why these shouldn't still be template bound and then given these values as defaults in the Style setters like the other properties? Otherwise it's even harder to customize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that we have to worry about positioning two 'areas' of the control separately: The Header and the Content. There are cases where you want the Header left aligned for example, but the actual content of the expander stretched.

What is done now is the Horizontal/VerticalContentAlignment is template bound to the content presenter. If we template bound in both areas we are forcing the header/content to always be in sync. Instead, stretching the 'container' elements allows independent positing of child content elements in either the header or the content area while also still supporting horizontal stretch.

Adding lightweight styling resources separate for the header/content area is also possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we should template bind the most common one that developers would want to customize and expose the other as theme resources so they can be customized as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experience tells me all properties of the Expander needs to be template bound to the content area rather than the header area. It will be most intuitive that way but it's opposite of how the code currently functions so additional changes are needed. (Background/Padding, etc. map to the header) We can expose theme resources for these header properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I should add that WPF appears to map some properties to both the header and the content areas simultaneously. I'm not sure I agree with that approach and think we should keep them separate. This is just FYI.

Copy link
Contributor

@StephenLPeters StephenLPeters Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree they should be seperate and generally we expect these properties to apply to the content area. So the control's background property should set the background of the content we provide to the control in its (metadata marked) content property.


In reply to: 590643542 [](ancestors = 590643542)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. But the other ones should at least be controlled by static/theme resources to provide better customization. Expander is odd because we usually see folks wanting to customize the header itself compared to the content, as the content is pretty much all end-developer driven vs. any chrome provided by the control itself at least in the default templates we've used in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the other ones should at least be controlled by static/theme resources to provide better customization

Yes, I'll add the lightweight styling resources so these can be customized without re-templating. There is well established convention for this now (but we are creating a lot of named resources indicating the styling system is deficient...)

Expander is odd because we usually see folks wanting to customize the header itself compared to the content,

I can see that point. Also keep in mind that the header is also one of the places the developer should customize the least. The design of the header is really set by the fluent design system and shouldn't change much so that end-users recognize it. It's like a ComboBox in my mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two resources to control the header's content alignment: ExpanderHeaderHorizontalContentAlignment and ExpanderHeaderVerticalContentAlignment. I am leaving the HorizontalAlignment of the header itself to stretched. This ensures it fits the grid which is the root for sizing purposes. Child content can still be aligned any way you would like (not only stretched).

@ranjeshj ranjeshj requested a review from jevansaks March 5, 2021 14:16
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

robloo added 7 commits March 12, 2021 16:04
- Header and content resources are clearly named and separated
- Relevant control properties now apply to the content area instead of header
- Border thickness of the content area is no longer controlled by the BorderThickness DP of the control itself
- This is required as BorderThickness of the control has local precedence over the visual state setter.
- The correct solution for this is another filter converter for border thickness
BorderBrush="{ThemeResource ExpanderDropDownBorderBrush}"
BorderThickness="{StaticResource ExpanderDropdownDownBorderThickness}"
BorderBrush="{TemplateBinding BorderBrush}"
BorderThickness="{StaticResource ExpanderContentDownBorderThickness}"
Copy link
Contributor Author

@robloo robloo Mar 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ideal.

  • BorderThickness="{StaticResource ExpanderContentDownBorderThickness}" should be
  • BorderThickness="{TemplateBinding BorderThickness}"

However, that would not work due to the BorderThickness of the control having local precedence (higher precedence than the visual state setter). The visual state is being used to switch between border thicknesses that have the top or bottom edge set to zero depending on the expand direction.

The correct solution to this is to add another filter converter for the border thickness similar to the CornerRadius. I expect push back so I didn't implement that.

What is done now is considered a hack but is usable for an initial release. Developers can still customize the thickness of the content area using ExpanderContentDownBorderThickness and ExpanderContentUpBorderThickness even while the BorderThickness of the control is ignored.

@robloo
Copy link
Contributor Author

robloo commented Mar 13, 2021

This is ready for review and merge.

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

<Setter Property="BorderBrush" Value="{ThemeResource ExpanderBorderBrush}" />
<Setter Property="Padding" Value="{StaticResource ExpanderHeaderPadding}" />
<Setter Property="HorizontalAlignment" Value="Stretch" />
<Setter Property="HorizontalContentAlignment" Value="Left" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this HorizontalContentAlignment no longer needed? Looking at this change it looks like the template never actually used this before. Is this just deleting unused lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4394 (comment)

  1. The default actually should be center per Expander Fixes #4394 (comment)
  2. I followed convention of the Button template
  3. When the default value of 'Center' is required, the property is simply omitted. This is better as all controls will fallback to the same default so if once in 1000 years the default was actually changed you wouldn't need to go and update all the templates (single source of truth). The rule of thumb: only change what you need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tashatitova Did I misunderstand about the HorizontalContentAlignment? Were you only referring to VerticalContentAlignment in that comment referenced above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tashatitova Can you confirm about HorizontalContentAlignment? The more I think about it, I think this was my misunderstanding and you were only talking about VerticalContentAlignment. I would like to make this change and get this PR merged ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just re-read it and your last comment emphasizes vertical. I'm going to make the change and add back HorizontalContentAlignment. Horizontal seems like it should be Left so @StephenLPeters was right to point this out.


<!-- Content -->
<StaticResource x:Key="ExpanderContentBackground" ResourceKey="CardBackgroundFillColorSecondaryBrush" />
<StaticResource x:Key="ExpanderContentBorderBrush" ResourceKey="ExpanderHeaderBorderBrush" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that this works? I'm worried that if you override the ExpanderHeaderBorderBrush at the local scope the ExpanderContentBorderBrush wont see your override. This is because the resource lookup doesn't restart when a resource redirection is encountered. So when the template is expanded and this resource is found, we'll start looking for a value for the ExpanderHeaderBorderBrush from our current position in the tree walk, which will miss the local overrides lower in the tree.

Copy link
Contributor Author

@robloo robloo Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that this works

No, I have not tested this specifically and didn't have a reason to at the time. My changes here were only renaming and I copied the original functionality. Code before renaming is below:

<StaticResource x:Key="ExpanderBorderBrush" ResourceKey="CardStrokeColorDefaultBrush" />

<StaticResource x:Key="ExpanderDropDownBackground" ResourceKey="CardBackgroundFillColorSecondaryBrush" />
<StaticResource x:Key="ExpanderDropDownBorderBrush" ResourceKey="ExpanderBorderBrush" />

ExpanderBorderBrush renamed to ExpanderHeaderBorderBrush
ExpanderDropDownBorderBrush renamed to ExpanderContentBorderBrush

Resource resolution is tricky to understand for those outside the codebase and is not well documented at least that I've found. So I'll take your word for how it functions in this area. If you want me to make the change that's fine. It will just be set to CardStrokeColorDefaultBrush as well.

Edit:

I'm worried that if you override the ExpanderHeaderBorderBrush at the local scope the ExpanderContentBorderBrush wont see your override

Actually, that's probably not a bad thing. If you change the Header resource one wouldn't expect the Content to change with it. Either way this line of code should be modified as indicated above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note ExpanderHeaderDisabledBorderBrush is also affected but would make sense to follow the ExpanderHeaderBorderBrush when changed. Still, this should change as well due to potential resource lookup issues.

<StaticResource x:Key="ExpanderHeaderDisabledBorderBrush" ResourceKey="ExpanderHeaderBorderBrush" />

robloo added 2 commits March 16, 2021 01:03
There is concern these would not be re-evaluated when expected
@robloo
Copy link
Contributor Author

robloo commented Mar 25, 2021

@StephenLPeters I see some commits that are going to cause a lot of merge issues. What/when is the plan for getting this merged?

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Expander team-Controls Issue for the Controls team
Projects
None yet
5 participants